Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] ICS20-V2 Path Forwarding #6693

Merged
merged 162 commits into from
Jun 28, 2024
Merged

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Jun 25, 2024

Description

Path forwarding feature for ICS20-V2 🚀

ref: #6536


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

chatton and others added 30 commits April 8, 2024 13:00
* chore: adding proto files for ics20-v2

* chore: add newline
* add CurrentVersion, EscrowVersion, update tests

* update hardcoded transfer channel version from interchaintest
---------

Co-authored-by: Charly <charly@interchain.berlin>
…shalling/conversion (#6226)

* chore: adding proto files for ics20-v2

* chore: add newline

* chore: modify MsgTransfer to accept coins instead of coin

* chore: reverted unintentional comment changes

* chore: reverted unintentional comment changes

* chore: adding test for conversion fn

* chore: fix e2e linter

* chore: adding docs

* chore: addressing PR feedback

* chore: remove duplicate import

* chore: addressing PR feedback

* chore: moved extration logic into internal package

* chore: commented out failing test

* chore: adding link to issue

* chore: remove duplicate import

* chore: fix linting errors

* FungibleTokenPacketData interface methods + tests

* linter

* wip: token validation

* update trace identifier validation in Token + tests

* rm Printf

* update with pr review

* add CurrentVersion, EscrowVersion, update tests

* pr review

* fix e2e tests

* pr review

* update e2e test version

* linter

* update hardcoded transfer channel version from interchaintest

* update hardcoded transfer channel version from interchaintest

* wip packet unmarshaller

* wip

* wip testing

* update test

* linter

* rm unnecessary version changes

* rm unnecessary artifacts

* update callbacks test

* update comment

* pr review

* rename getMultiDenomFungibleTokenPacketData to unmarshalPacketDataBytesToICS20V2

---------

Co-authored-by: chatton <github.qpeyb@simplelogin.fr>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…allbacks (#6175)

* add SupportedVersions array for different ics20 versions, add version checking on channel handshake application callbacks

* add tests

* update pr review

* pr review

* last few pr review nits

* linter

* add version counter proposing

* fix missing app versino

* update code + tests to return our proposed version if counterparty version is invalid

* remove if statement

* address review comments: return ics20-2 if counterparty version is not supported

---------

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…transfers (#6252)

* add transfer authz code + tests

* linter

* address review comments

---------

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…ks (#6189)

* chore: adding proto files for ics20-v2

* chore: add newline

* chore: modify MsgTransfer to accept coins instead of coin

* chore: reverted unintentional comment changes

* chore: reverted unintentional comment changes

* chore: adding test for conversion fn

* chore: fix e2e linter

* chore: adding docs

* chore: addressing PR feedback

* chore: remove duplicate import

* chore: addressing PR feedback

* chore: moved extration logic into internal package

* chore: commented out failing test

* chore: adding link to issue

* chore: remove duplicate import

* chore: fix linting errors

* FungibleTokenPacketData interface methods + tests

* linter

* wip: token validation

* update trace identifier validation in Token + tests

* rm Printf

* update with pr review

* pr review

* linter

* rm unused function: linter

* wip pr feedback

* fix test

* pr review

* lintttttt

* wip: backwards compatibility for transfer rpc

* implement changes for ics20-v2 packet data for onRecvPacket, onAcknowledgePacket and onTimeoutPacket

* fix callbacks tests

* lint

---------

Co-authored-by: chatton <github.qpeyb@simplelogin.fr>
Co-authored-by: Charly <charly@interchain.berlin>
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the non-test files and acking for merge.

I do have some nits on structuring. In general, I found the use of helper functions too extensive, to the point where it hurt understandability more than it helped. Left comments where relevant. This can be discussed and fixed in later PRs

Acking to unblock since i mainly wanted to review the overall logic in key places. I can review test files as well another day if folks want me to

modules/apps/callbacks/ibc_middleware_test.go Show resolved Hide resolved
modules/apps/transfer/client/cli/tx.go Outdated Show resolved Hide resolved
modules/apps/transfer/keeper/forwarding.go Outdated Show resolved Hide resolved
modules/apps/transfer/keeper/relay.go Show resolved Hide resolved
modules/apps/transfer/keeper/relay.go Show resolved Hide resolved
modules/apps/transfer/keeper/relay.go Show resolved Hide resolved
modules/apps/transfer/keeper/relay.go Outdated Show resolved Hide resolved
modules/apps/transfer/types/forwarding.go Outdated Show resolved Hide resolved
Co-authored-by: Aditya <14364734+AdityaSripal@users.noreply.github.com>
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo!! Reviewed the core logic and left any relevant notes. I skimmed over the tests, but largely relying on the fact that we are checking code cov and have already reviewed all the files changed. Happy to see this merged and followup issues addressed after the fact

modules/apps/transfer/keeper/relay.go Outdated Show resolved Hide resolved
modules/apps/transfer/keeper/relay.go Outdated Show resolved Hide resolved
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks everyone for their work on this feature! ❤️

modules/apps/transfer/keeper/msg_server.go Show resolved Hide resolved
modules/apps/transfer/keeper/relay.go Show resolved Hide resolved

ParamsKey = "params"

KeyPacketForwardPrefix = "forwardedPacket"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's group them together in a follow up PR

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@chatton chatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't leave an approval since I created the PR but I can leave a 🟢 .

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely amazing job everyone!

Going to leave for tomorrow's early bird to merge, whomever that might be

@crodriguezvega
Copy link
Contributor

I am looking at the upgrade tests that fail to see if I can do a quick fix before merging.

Copy link

sonarcloud bot commented Jun 28, 2024

@crodriguezvega crodriguezvega added this pull request to the merge queue Jun 28, 2024
Merged via the queue into main with commit 1812138 Jun 28, 2024
131 of 146 checks passed
@crodriguezvega crodriguezvega deleted the feat/ics20-v2-path-forwarding branch July 9, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20-transfer type: feature New features, sub-features or integrations
Projects
Status: Done 🥳
Development

Successfully merging this pull request may close these issues.